Use token list of accounts for user authorization #240

gcampo88 · 2017-01-31T19:18:10Z

extracts account authorization information from the auth token in the ApiAuthenticated concern
uses that list of account IDs instead of querying the memberships table to determine which accounts to use
updates StubToken class in test_helper.rb to more closely mimic auth token behavior in terms of authorized_resources list

…accounts.

codecov · 2017-01-31T19:18:11Z

Codecov Report

❗ No coverage uploaded for pull request head (fix/token-accounts@5cb4e8b).

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78368f5...5cb4e8b. Read the comment docs.

houndci-bot · 2017-01-31T19:19:34Z

test/test_helper.rb

@@ -74,7 +76,7 @@ def initialize(res, scopes, explicit_user_id = nil)
  end

  def authorized?(r, s = nil)
-    resource == r.to_s && (s.nil? || scopes.include?(s.to_s))
+    authorized_resources.keys.include?(r) && (s.nil? || authorized_resources.values.flatten.include?(s.to_s))


Line is too long. [109/100]

houndci-bot · 2017-01-31T19:19:34Z

test/controllers/api/authorizations_controller_test.rb

@@ -3,7 +3,7 @@
 describe Api::AuthorizationsController do

  let (:user) { create(:user) }
-  let (:token) { OpenStruct.new.tap { |t| t.user_id = user.id } }
+  let (:token) { StubToken.new(user.individual_account.id, "admin", user.id) }


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-31T19:19:34Z

test/controllers/api/auth/accounts_controller_test.rb

+        member_account.id => "member",
+        individual_account.id => "admin"
+      }
+    }


} at 17, 4 is not aligned with StubToken.new(nil, nil, user.id).tap { |t| at 12, 17 or let (:token) { StubToken.new(nil, nil, user.id).tap { |t| at 12, 2.

houndci-bot · 2017-01-31T19:19:35Z

test/controllers/api/auth/accounts_controller_test.rb

+  let (:token) { StubToken.new(nil, nil, user.id).tap { |t|
+      t.authorized_resources = {
+        member_account.id => "member",
+        individual_account.id => "admin"


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-31T19:19:35Z

test/controllers/api/auth/accounts_controller_test.rb

+
+  let (:token) { StubToken.new(nil, nil, user.id).tap { |t|
+      t.authorized_resources = {
+        member_account.id => "member",


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-01-31T19:19:35Z

test/controllers/api/auth/accounts_controller_test.rb

+  let (:member_account) { create(:account) }
+  let (:unapproved_account) { create(:account)}
+
+  let (:token) { StubToken.new(nil, nil, user.id).tap { |t|


Avoid using {...} for multi-line blocks.
Block body expression is on the same line as the block start.

houndci-bot · 2017-01-31T19:19:36Z

test/controllers/api/auth/accounts_controller_test.rb

  let (:random_account) { create(:account) }
-  before { unapproved_account.memberships.first.update!(approved: false) }
+  let (:member_account) { create(:account) }
+  let (:unapproved_account) { create(:account)}


Space missing inside }.

houndci-bot · 2017-01-31T19:19:36Z

app/models/user.rb

-    Series.
-      joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `series`.`account_id`').
-      where(['memberships.user_id = ? and memberships.approved is true', id])
+    !approved_accounts.nil? ? Series.where({ account_id: approved_accounts.try(:ids) }) : []


Redundant curly braces around a hash parameter.

houndci-bot · 2017-01-31T19:19:37Z

app/models/user.rb

-    Story.
-      joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `pieces`.`account_id`').
-      where(['memberships.user_id = ? and memberships.approved is true', id])
+    !approved_accounts.nil? ? Story.where({ account_id: approved_accounts.try(:ids) }) : []


Redundant curly braces around a hash parameter.

houndci-bot · 2017-01-31T19:19:37Z

app/controllers/concerns/api_authenticated.rb

  end

  def authenticate_user!
    user_not_authorized unless current_user
  end

+  def get_authorized_resources
+    if prx_auth_token.authorized_resources
+      current_user.approved_accounts ||= Account.where({ id: prx_auth_token.authorized_resources.keys })


Redundant curly braces around a hash parameter.
Line is too long. [104/100]

kookster

I think I want a 2nd opinion on this one from Ryan, as he also worked on these authenticated controllers. @cavis can you take a look see at the code and my concerns?

kookster · 2017-01-31T23:19:33Z

app/controllers/concerns/api_authenticated.rb

+  def get_authorized_resources
+    token_accounts = prx_auth_token.authorized_resources.try(:keys)
+    if token_accounts
+      current_user.approved_accounts ||= Account.where(id: token_accounts)


I'm not sure we want the user model, which is retrieved from the DB and is in the model layer, also aware of the token and its contents which come from the controller layer. Seems like we are having controller / request logic and information leak into the model which should not be aware of or rely on that layer.

The other part of this is that it is could be misleading, because the user may have a different set of accounts they can access than is allowed by and listed in the token.

For example, you could generate a token with only a subset of the actual accounts the user has access to, or using client credentials, get a different account entirely in aur than is reflected in the user's account membership.

So the more I think about it, it seems like with the move to get authorized accounts from the token, not the user, they become 2 different things - there is the user that logged in, and the list of accounts authorized in the token, and they are not necessarily related except for having both been identified in the token.

Curious what @cavis might say about this as well - with the separation of accounts and users, where the token provides the list of accounts not the user, I think it makes sense to move these methods using the list of accounts out of the user model, perhaps into methods on the ApiAuthenticated concern, or just in the relevant controller?

kookster · 2017-01-31T23:24:44Z

app/models/user.rb

-    Story.
-      joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `pieces`.`account_id`').
-      where(['memberships.user_id = ? and memberships.approved is true', id])
+    !approved_accounts.nil? ? Story.where(account_id: approved_accounts.try(:ids)) : []


could Story.where(account_id: approved_accounts.try(:ids)) be in a controller method instead of in the user model? doesn't seem to be using anything in the user model, except the convenience that it is the resource loaded by authorized controllers.

Could certainly be in there! I'm not quite sure where to place the method definitions for approved_account_series and approved_account_stories because they are called in the representers, which would lead me to want to put them in the model layer, but also are called in the controller...

yeah, that is annoying - looks like they do get used for counts in the authorization_representer, but only that one representer at least.

We can talk about it when I get in - one way to handle this may be to create an authorization resource based on the token, which could encompass both access to the user, and the list of accounts. That's sorta what feeder does, if you have a look at that, where the auth controller has this for the resource:

def resource Authorization.new(prx_auth_token) end

And then the authorization model is basically a wrapper around the token, accessing the related user(well, user_id, feeder would have to make a remote call to getthe user), and could also return the approved_accounts and such:

class Authorization include HalApi::RepresentedModel attr_accessor :token def initialize(token) @token = token end def user_id token.user_id end ... end

That would separate the idea of what info comes from the token and it's list of accounts vs. the user and the list of all member accounts, might be better?

…ed on token account IDs

houndci-bot · 2017-02-01T18:29:15Z

app/models/authorization.rb

+    get_authorized_resources
+  end
+
+  def get_authorized_resources


Put empty method definitions on a single line.

houndci-bot · 2017-02-01T18:29:16Z

app/controllers/api/auth/stories_controller.rb

@@ -47,7 +47,7 @@ def create_resource
      story.creator_id = current_user.id
      story.account_id ||= story.series.try(:account_id)
      story.account_id ||= current_user.account_id
-      story.account_id ||= current_user.approved_accounts.first.try(:id)
+      story.account_id ||= current_user.approved_accounts.first.try(:id) # not sure if I should change this to look at token


Line is too long. [124/100]

This is in the stories controller -- not sure whether to change this to just use the first account ID listed on the auth token, or perhaps the first account ID with role "admin", or else to leave it as is -- calling the User model, which joins against memberships. (Or should this simply be the user's individual_account ? )

So this is the fallback for the fallback (when there is neither a series.account nor a default user account), so I think it's fine to fallback on the first account.

Creating a story doesn't require :admin permissions, so I don't think we need to check that.

There is another issue here though which is that the default account or series account may not be in the token's list of accounts, so we should check for that too - we shouldn't set the account to a value that the token does not include, does that make sense?

gcampo88 · 2017-02-01T18:38:22Z

@kookster this more along the lines of what you're thinking?

kookster · 2017-02-02T20:21:03Z

app/models/authorization.rb

@@ -0,0 +1,23 @@
+# encoding: utf-8
+
+class Authorization


I like this whole class.

kookster

couple of things, smallish

kookster · 2017-02-02T21:30:47Z

test/models/user_test.rb

-    create(:account, user: user, stories_count: 2)
-    user.approved_account_stories.count.must_equal start_count + 2
+  it 'has a list of stories and series' do
+    user.approved_account_stories.must_include story


Are you leaving the User methods like approved_account_stories, even though they aren't being used anymore?

approved_account_stories and approved_account_series are both used in the Authorization Representer. Do you think that representer should be looking at the stories + series associated with the accounts on the token instead?

houndci-bot · 2017-02-03T21:39:08Z

test/controllers/concerns/api_authenticated_test.rb

@@ -12,6 +12,9 @@ def current_user
    def user_not_authorized
      raise 'user_not_authorized'
    end
+
+    def prx_auth_token


Put empty method definitions on a single line.

houndci-bot · 2017-02-03T21:39:09Z

test/representers/api/authorization_representer_test.rb

@@ -4,7 +4,9 @@

  let(:user) { create(:user) }
  let(:account) { user.default_account }
-  let(:representer) { Api::AuthorizationRepresenter.new(user) }
+  let(:token) { StubToken.new(account.id, 'admin', user.id)}


Space missing inside }.

houndci-bot · 2017-02-03T21:39:09Z

app/controllers/api/auth/stories_controller.rb

    end
  end

  def create_resource
    super.tap do |story|
      story.creator_id = current_user.id
      story.account_id ||= story.series.try(:account_id)
-      story.account_id ||= current_user.account_id
-      story.account_id ||= current_user.approved_accounts.first.try(:id)
+      story.account_id ||= current_user.account_id if authorization.authorized?(current_user.default_account)


Line is too long. [109/100]

thanks for adding that check - clever to use the authorized? method, I was thinking of doing a detect on the account list - this is much better

gcampo88 · 2017-02-03T21:43:15Z

app/models/authorization.rb

+    @token = token
+  end
+
+  def id


I added this because the AuthorizationRepresenter wants an id to build links ... misguided?

what does it want the id for? the self link? I don't think you should need to provide an id for the representer, can we track this down?

if it is the self link, which is all I can think of, we can override the self_link method on the representer to not need this.

bug in hal gem - PRX/hal_api-rails#8

gcampo88 · 2017-02-03T21:43:40Z

app/models/authorization.rb

+    default_account.id
+  end
+
+  def default_account


similarly, added this because AuthorizationRepresenter wants to return a default_account

yeah, makes sense to me

gcampo88 · 2017-02-03T21:46:10Z

ok, back to you @kookster !

kookster

I'm curious if you can get rid of needing the id attribute on the Authorization model and still make the representer happy.

kookster · 2017-02-09T22:43:14Z

app/controllers/api/auth/stories_controller.rb

    end
  end

  def create_resource
    super.tap do |story|
      story.creator_id = current_user.id
      story.account_id ||= story.series.try(:account_id)
-      story.account_id ||= current_user.account_id
-      story.account_id ||= current_user.approved_accounts.first.try(:id)
+      story.account_id ||= current_user.account_id if authorization.authorized?(current_user.default_account)


thanks for adding that check - clever to use the authorized? method, I was thinking of doing a detect on the account list - this is much better

kookster · 2017-02-09T22:44:43Z

app/models/authorization.rb

+    @token = token
+  end
+
+  def id


what does it want the id for? the self link? I don't think you should need to provide an id for the representer, can we track this down?

kookster · 2017-02-09T22:45:19Z

app/models/authorization.rb

+    @token = token
+  end
+
+  def id


if it is the self link, which is all I can think of, we can override the self_link method on the representer to not need this.

kookster · 2017-02-09T22:45:39Z

app/models/authorization.rb

+    default_account.id
+  end
+
+  def default_account


yeah, makes sense to me

kookster · 2017-02-09T22:47:24Z

app/models/authorization.rb

+  end
+
+  def token_auth_stories
+    Story.where(account_id: token_auth_accounts.try(:ids)) unless token_auth_accounts.nil?


do you need the try when you have the unless nil? already? Fine to leave it this way, I am a belt and suspenders kind of person myself.

just doing belt + suspenders!

kookster · 2017-02-09T22:49:59Z

test/models/authorization_test.rb

+  end
+
+  it 'checks against token to see if accounts are authorized' do
+    authorization.authorized?(account).must_equal true


I believe you could also write this as:
authorization.must_be :authorized?, account

just a point of information - this test is great

gcampo88 added 2 commits January 30, 2017 17:30

Use the list of account IDs on the token to determine user access to …

b11c0f0

…accounts.

Get token-authorized accounts in ApiAuth concern, update tests

862f874

houndci-bot reviewed Jan 31, 2017

View reviewed changes

gcampo88 added 2 commits January 31, 2017 14:48

hound

cde1998

Remove redundant test

5cb4e8b

gcampo88 changed the title ~~WIP use token list of accounts for user authorization~~ Use token list of accounts for user authorization Jan 31, 2017

gcampo88 requested a review from kookster January 31, 2017 20:57

kookster requested changes Jan 31, 2017

View reviewed changes

kookster requested a review from cavis January 31, 2017 23:39

Create authorization model to query accounts, series, and stories bas…

a1d1bb6

…ed on token account IDs

houndci-bot reviewed Feb 1, 2017

View reviewed changes

gcampo88 added 2 commits February 1, 2017 13:32

Oops

ad87223

Remove newlines

c9ade9b

kookster reviewed Feb 2, 2017

View reviewed changes

app/models/authorization.rb

@@ -0,0 +1,23 @@

# encoding: utf-8

class Authorization

Copy link

Member

kookster Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this whole class.

kookster requested changes Feb 2, 2017

View reviewed changes

Nix user model methods and put everything in authorization

3dcd64a

houndci-bot reviewed Feb 3, 2017

View reviewed changes

hound

dded0e4

gcampo88 commented Feb 3, 2017

View reviewed changes

unused test objects

af9cafb

kookster requested changes Feb 9, 2017

View reviewed changes

kookster approved these changes Feb 10, 2017

View reviewed changes

kookster merged commit 6f2b67b into master Feb 10, 2017

kookster deleted the fix/token-accounts branch February 10, 2017 00:06

gcampo88 mentioned this pull request Feb 15, 2017

Use the list of accounts on the token for authorization, not all those associated with the user #216

Closed

3 tasks

Use token list of accounts for user authorization #240

Use token list of accounts for user authorization #240

Conversation

gcampo88 commented Jan 31, 2017

codecov bot commented Jan 31, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kookster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kookster Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kookster Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kookster Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

gcampo88 commented Feb 1, 2017

Choose a reason for hiding this comment

kookster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcampo88 commented Feb 3, 2017

kookster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2017 •

edited

Loading

kookster Jan 31, 2017 •

edited

Loading

kookster Feb 1, 2017 •

edited

Loading

kookster Feb 2, 2017 •

edited

Loading